Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: refactor with typescript to support esm and cjs both #56

Merged
merged 10 commits into from
Dec 8, 2024

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented May 29, 2024

BREAKING CHANGE: Drop Node.js < 16 support

  1. 使用 ts 重构
  2. 使用 tshy 支持 esm 和 cjs
  3. test 使用 test-runner (这里需要 node v18 版本)

merge from #51

Summary by CodeRabbit

  • New Features

    • Introduced a new waitPort function to asynchronously wait for a specified port to become available.
    • Added a new ESLint configuration to enforce TypeScript linting rules.
  • Bug Fixes

    • Reverted a feature in the detect-port package due to issues raised.
  • Documentation

    • Updated README.md for improved clarity and updated badge links.
    • Modified CONTRIBUTING.md to reflect changes in testing commands.
  • Chores

    • Introduced a new TypeScript configuration file (tsconfig.json).
    • Updated package.json to reflect changes in dependencies and project structure.
  • Tests

    • Added comprehensive tests for the new waitPort and updated tests for the CLI and detectPort function.

Copy link

coderabbitai bot commented May 29, 2024

Warning

Rate limit exceeded

@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 10 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 74f08a4 and 8bf8903.

Walkthrough

The pull request introduces a new ESLint configuration file, updates the changelog, modifies the contribution guidelines, and restructures the README file. It also includes significant changes to the package.json to reflect new module paths and dependencies, transitions several files to ES module syntax, and adds a new wait-port.ts file that provides functionality for waiting on a port. Additionally, various test files are created or updated to ensure proper functionality of the new and modified features.

Changes

File Change Summary
.eslintrc New ESLint configuration file created, extending eslint-config-egg/typescript and eslint-config-egg/lib/rules/enforce-node-prefix.
CHANGELOG.md Updated to reflect changes in versions 1.6.1 and 1.6.0 of the detect-port package, including a revert of a feature and detailed history of previous versions.
CONTRIBUTING.md Modified testing instructions to change the command from $ make test to $ npm test.
HISTORY.md Deleted file containing a detailed changelog for the detect-port project.
README.md Updated badge links, removed logo section, simplified usage instructions, and restructured sections for clarity.
package.json Significant updates including changes to bin, main, files, removal of debug, updates to address, changes in devDependencies, and new scripts. Added engines, types, type, module, and exports fields.
src/detect-port.ts Transitioned to ES module syntax, updated function signatures for detectPort, tryListen, and listen, improved error handling, and added TypeScript type annotations.
src/index.ts Introduced new imports for detectPort and waitPort, exporting them as default and named exports.
src/wait-port.ts New file that implements waitPort, which waits for a specified port to become available, including a utility function for sleep and logging functionality.
test/cli.test.ts Introduced tests for the CLI tool, validating version reporting, usage information, and port availability.
test/detect-port.test.ts Added unit tests for the detectPort function, covering various scenarios for port detection.
test/wait-port.test.ts New test file for waitPort, validating its functionality and error handling.
tsconfig.json New TypeScript configuration file created, extending base configuration from @eggjs/tsconfig with specified compiler options.
src/bin/detect-port.ts Transitioned to ES module syntax, updated removeByValue function with type annotations, and improved file handling.

Poem

🐇 In the land of code where rabbits play,
New configs and tests brighten the day.
With ports that wait and linting so fine,
Our project hops forward, a joyful design!
So let’s code and cheer, with each little change,
For every new feature, our hearts rearrange! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

socket-security bot commented May 29, 2024

Copy link

socket-security bot commented May 29, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Outside diff range and nitpick comments (2)
bin/detect-port.ts (1)

Line range hint 49-49: Replace isNaN with Number.isNaN for more accurate checks that avoid type coercion.

- } else if (isNaN(port)) {
+ } else if (Number.isNaN(port)) {
src/detect-port.ts (1)

Line range hint 19-47: Refactor to avoid reassigning function parameters.

Reassigning function parameters can lead to confusing bugs and harder-to-maintain code. Consider using local variables instead.

- export default function detectPort(port?: number | string | PortConfig | DetectPortCallback, callback?: DetectPortCallback) {
+ export default function detectPort(inputPort?: number | string | PortConfig | DetectPortCallback, inputCallback?: DetectPortCallback) {
+   let port = inputPort;
+   let callback = inputCallback;
Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 5e8f3ed and b7bcccf.
Files selected for processing (13)
  • .eslintrc (1 hunks)
  • .github/workflows/nodejs.yml (1 hunks)
  • .github/workflows/release.yml (1 hunks)
  • .gitignore (1 hunks)
  • bin/detect-port.ts (3 hunks)
  • package.json (2 hunks)
  • src/detect-port.ts (5 hunks)
  • src/index.ts (1 hunks)
  • src/wait-port.ts (1 hunks)
  • test/cli.test.ts (1 hunks)
  • test/detect-port.test.ts (1 hunks)
  • test/wait-port.test.ts (1 hunks)
  • tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (4)
  • .eslintrc
  • .github/workflows/release.yml
  • .gitignore
  • tsconfig.json
Additional Context Used
Biome (29)
bin/detect-port.ts (2)

31-31: Use Number.parseInt instead of the equivalent global.


49-49: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

src/detect-port.ts (15)

49-49: Unexpected any. Specify a different type.


64-64: Unexpected any. Specify a different type.


94-94: Unexpected any. Specify a different type.


112-112: Unexpected any. Specify a different type.


119-119: Unexpected any. Specify a different type.


24-24: Reassigning a function parameter is confusing.


28-28: Reassigning a function parameter is confusing.


32-32: Reassigning a function parameter is confusing.


23-23: Reassigning a function parameter is confusing.


27-27: Reassigning a function parameter is confusing.


32-32: Use Number.parseInt instead of the equivalent global.


51-51: Reassigning a function parameter is confusing.


54-54: Reassigning a function parameter is confusing.


55-55: Reassigning a function parameter is confusing.


128-128: Reassigning a function parameter is confusing.

src/wait-port.ts (3)

21-21: Unexpected any. Specify a different type.


22-22: Unexpected any. Specify a different type.


14-14: Use Number.Infinity instead of the equivalent global.

test/cli.test.ts (3)

51-51: Template literals are preferred over string concatenation.


45-45: Use Number.parseInt instead of the equivalent global.


52-52: Use Number.parseInt instead of the equivalent global.

test/detect-port.test.ts (4)

51-51: Prefer for...of instead of forEach.


84-84: Unexpected any. Specify a different type.


90-90: Unexpected any. Specify a different type.


103-103: Template literals are preferred over string concatenation.

test/wait-port.test.ts (2)

11-11: Prefer for...of instead of forEach.


28-28: Unexpected any. Specify a different type.

GitHub Check Runs (3)
Node.js / Test (ubuntu-latest, 22) failure (2)

bin/detect-port.ts: [failure] 8-8:
The 'import.meta' meta-property is only allowed when the '--module' option is 'es2020', 'es2022', 'esnext', 'system', 'node16', or 'nodenext'.

Node.js / Test (ubuntu-latest, 20) failure (2)

bin/detect-port.ts: [failure] 8-8:
The 'import.meta' meta-property is only allowed when the '--module' option is 'es2020', 'es2022', 'esnext', 'system', 'node16', or 'nodenext'.

Node.js / Test (ubuntu-latest, 18) failure (2)

bin/detect-port.ts: [failure] 8-8:
The 'import.meta' meta-property is only allowed when the '--module' option is 'es2020', 'es2022', 'esnext', 'system', 'node16', or 'nodenext'.

Additional comments not posted (5)
src/index.ts (2)

1-2: The imports are correctly updated to use ES module syntax.


4-6: The export statements are correctly updated to support both default and named exports, aligning with the PR's objectives to support ESM and CJS.

.github/workflows/nodejs.yml (1)

12-15: The workflow file has been updated to use the latest versions of Node.js, ensuring compatibility with the new features and dropping support for older versions as per the PR description.

package.json (1)

13-13: The updates to the package.json align with the PR's objectives, including the update of dependencies and scripts to support TypeScript and the new module structure.

Also applies to: 24-39, 42-74

bin/detect-port.ts (1)

8-8: Ensure the TypeScript configuration allows the use of import.meta. This might require setting the module option to nodenext or similar in tsconfig.json.

Verification successful

The TypeScript configuration in tsconfig.json has the module option set to nodenext, which supports the use of import.meta.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the TypeScript configuration for module option.

# Test: Search for the TypeScript configuration. Expect: Module option set to support 'import.meta'.
cat tsconfig.json | jq '.compilerOptions.module'

Length of output: 59

test/wait-port.test.ts Outdated Show resolved Hide resolved
test/wait-port.test.ts Show resolved Hide resolved
src/wait-port.ts Show resolved Hide resolved
src/wait-port.ts Outdated Show resolved Hide resolved
bin/detect-port.ts Outdated Show resolved Hide resolved
src/detect-port.ts Show resolved Hide resolved
src/detect-port.ts Show resolved Hide resolved
test/detect-port.test.ts Outdated Show resolved Hide resolved
test/detect-port.test.ts Show resolved Hide resolved
test/detect-port.test.ts Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (9)
src/detect-port.ts (1)

94-94: Fix typographical error in the comment.

In the comment, correct the word "unkonwn" to "unknown" for proper spelling.

src/wait-port.ts (1)

6-6: Use built-in setTimeout from node:timers/promises instead of custom sleep function.

Node.js 16+ provides a Promise-based setTimeout in node:timers/promises, which simplifies the code and removes the need for a custom sleep function.

Apply this diff to refactor:

- const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));
+ import { setTimeout as sleep } from 'node:timers/promises';
test/wait-port.test.ts (2)

13-13: Improve test case description for clarity.

The test description 'should be work' is grammatically incorrect. Consider rephrasing it to 'should work' or 'should wait for the port successfully'.

Apply this diff to update the test description:

- it('should be work', async () => {
+ it('should wait for the port successfully', async () => {

25-25: Improve test case description for clarity.

The test description 'should be work when retries exceeded' is unclear. Rephrase it to 'should throw an error when retries are exceeded' for better understanding.

Apply this diff to update the test description:

- it('should be work when retries exceeded', async () => {
+ it('should throw an error when retries are exceeded', async () => {
bin/detect-port.cjs (2)

Line range hint 36-44: Consider adding error handling for the callback

The error handling in the callback could be improved to ensure proper process exit codes on errors.

 detectPort(random, (err, port) => {
   if (isVerbose) {
     if (err) {
       console.log(`get available port failed with ${err}`);
+      process.exit(1);
     }
     console.log(`get available port ${port} randomly`);
   } else {
     console.log(port || random);
   }
 });

Line range hint 65-80: Consider modernizing the callback pattern

Since this PR requires Node.js 16+, consider using async/await pattern for better readability and error handling.

-  detectPort(port, (err, _port) => {
+  (async () => {
+    try {
+      const _port = await detectPort(port);
       if (isVerbose) {
-        if (err) {
-          console.log(`get available port failed with ${err}`);
-        }
         if (port !== _port) {
           console.log(`port ${port} was occupied`);
         }
         console.log(`get available port ${_port}`);
       } else {
         console.log(_port || port);
       }
-    });
+    } catch (err) {
+      if (isVerbose) {
+        console.log(`get available port failed with ${err}`);
+      }
+      process.exit(1);
+    }
+  })();
test/cli.test.ts (1)

34-38: Uncomment and fix the random port test

The commented test for random port output should either be removed or uncommented and fixed. If it's meant to be skipped temporarily, add a TODO comment explaining why.

README.md (1)

Line range hint 41-60: Add ESM import example alongside CommonJS

Since the package now supports both ESM and CJS, the documentation should show both import styles.

Add ESM example:

 ```javascript
 const detect = require('detect-port');
 /**
  * use as a promise
  */
+```
+
+```javascript
+// ESM
+import detect from 'detect-port';
+```
CHANGELOG.md (1)

75-75: Fix hyphenation in changelog entry

The term "double check" should be hyphenated as "double-check" for grammatical correctness.

- * fix: should double check 0.0.0.0 and localhost (#20)
+ * fix: should double-check 0.0.0.0 and localhost (#20)
🧰 Tools
🪛 LanguageTool

[grammar] ~75-~75: The verb “double-check” is spelled with a hyphen.
Context: ...-11 ================== * fix: should double check 0.0.0.0 and localhost (#20) * docs: i...

(DOUBLE_HYPHEN)

🪛 Markdownlint (0.35.0)

75-75: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b7bcccf and 815eb45.

⛔ Files ignored due to path filters (1)
  • logo.png is excluded by !**/*.png
📒 Files selected for processing (14)
  • .eslintrc (1 hunks)
  • CHANGELOG.md (1 hunks)
  • CONTRIBUTING.md (1 hunks)
  • HISTORY.md (0 hunks)
  • README.md (4 hunks)
  • bin/detect-port.cjs (3 hunks)
  • package.json (1 hunks)
  • src/detect-port.ts (5 hunks)
  • src/index.ts (1 hunks)
  • src/wait-port.ts (1 hunks)
  • test/cli.test.ts (1 hunks)
  • test/detect-port.test.ts (1 hunks)
  • test/wait-port.test.ts (1 hunks)
  • tsconfig.json (1 hunks)
💤 Files with no reviewable changes (1)
  • HISTORY.md
✅ Files skipped from review due to trivial changes (3)
  • CONTRIBUTING.md
  • src/index.ts
  • .eslintrc
🧰 Additional context used
🪛 LanguageTool
README.md

[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 890 characters long)
Context: ...t [![NPM version][npm-image]][npm-url] CI [![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][sny...

(EN_EXCESSIVE_EXCLAMATION)

CHANGELOG.md

[grammar] ~75-~75: The verb “double-check” is spelled with a hyphen.
Context: ...-11 ================== * fix: should double check 0.0.0.0 and localhost (#20) * docs: i...

(DOUBLE_HYPHEN)

🪛 eslint
bin/detect-port.cjs

[error] 3-3: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)


[error] 4-4: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)


[error] 5-5: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)

🪛 Markdownlint (0.35.0)
CHANGELOG.md

20-20: Expected: atx; Actual: setext
Heading style

(MD003, heading-style)


26-26: Expected: atx; Actual: setext
Heading style

(MD003, heading-style)


45-45: Expected: atx; Actual: setext
Heading style

(MD003, heading-style)


55-55: Expected: atx; Actual: setext
Heading style

(MD003, heading-style)


67-67: Expected: atx; Actual: setext
Heading style

(MD003, heading-style)


72-72: Expected: atx; Actual: setext
Heading style

(MD003, heading-style)


24-24: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


30-30: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


33-33: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


36-36: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


37-37: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


38-38: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


39-39: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


40-40: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


41-41: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


42-42: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


43-43: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


49-49: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


52-52: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


53-53: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


59-59: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


60-60: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


63-63: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


64-64: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


65-65: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


70-70: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


75-75: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


76-76: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


80-80: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


84-84: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


88-88: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


89-89: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


93-93: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


97-97: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


101-101: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


105-105: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


109-109: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


23-23: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


29-29: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


32-32: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


35-35: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


48-48: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


51-51: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


58-58: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


62-62: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

🔇 Additional comments (12)
src/detect-port.ts (2)

32-32: Use Number.parseInt instead of parseInt for clarity.

Using Number.parseInt improves readability by explicitly indicating the method's association with the Number object.


64-64: Avoid casting errors to any; use a more specific error type.

Casting errors to any defeats TypeScript's type safety. Consider using NodeJS.ErrnoException for better type specificity.

Also applies to: 94-94, 119-119

tsconfig.json (1)

6-6: Ensure the target ES2022 is compatible with Node.js 16.

Since Node.js 16 supports up to ES2021 features, setting the target to ES2022 might introduce features not supported in Node.js 16. Consider adjusting the target to ES2021 or verify compatibility.

src/wait-port.ts (1)

21-23: Avoid using any; define a custom error type for better type safety.

Using any undermines TypeScript's type system. Define a custom error interface to include retries and count properties.

Apply this diff to define a custom error type:

+ interface RetryError extends Error {
+   retries: number;
+   count: number;
+ }
- const err = new Error('retries exceeded');
- (err as any).retries = retries;
- (err as any).count = count;
+ const err: RetryError = new Error('retries exceeded') as RetryError;
+ err.retries = retries;
+ err.count = count;
test/wait-port.test.ts (2)

10-10: Use for...of loop instead of forEach for better readability.

Replacing the forEach with a for...of loop enhances readability and maintains consistent iteration patterns.

Apply this diff to update the loop:

- servers.forEach(server => server.close());
+ for (const server of servers) {
+   server.close();
+ }

29-29: Avoid using any in the catch block; specify the error type.

Using any defeats type safety. Specify the error as Error to maintain strong typing.

Apply this diff to update the error type:

- } catch (err:any) {
+ } catch (err: Error) {
package.json (1)

68-70: Ensure compatibility between type, main, and exports configurations.

Setting "type": "module" while your main points to CommonJS may cause issues. Verify that the module, main, and exports fields are correctly configured to support both ESM and CommonJS environments.

bin/detect-port.cjs (1)

3-5: ESLint warnings about require() can be safely ignored

The .cjs extension explicitly indicates this is a CommonJS module, so using require() is appropriate here. The ESLint warnings can be suppressed as this is an intentional CommonJS file.

🧰 Tools
🪛 eslint

[error] 3-3: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)


[error] 4-4: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)


[error] 5-5: Require statement not part of import statement.

(@typescript-eslint/no-var-requires)

test/cli.test.ts (1)

42-42: Use template literals for string conversion

Replace string concatenation with template literals for better readability.

- const res = await execaNode(binFile, [ givenPort + '' ]);
+ const res = await execaNode(binFile, [ `${givenPort}` ]);
test/detect-port.test.ts (3)

1-8: LGTM! Modern import practices

The use of the node: prefix for built-in modules follows Node.js best practices and improves module resolution.


104-104: Use template literals for string interpolation

Replace string concatenation with template literals for better readability.

- it('work with listening next port 24001 because 24000 was listened to ' + ip(), async () => {
+ it(`work with listening next port 24001 because 24000 was listened to ${ip()}`, async () => {

53-55: 🛠️ Refactor suggestion

Improve server cleanup reliability

The server cleanup can be improved in two ways:

  1. Use for...of loop as suggested in previous reviews
  2. Add proper error handling and wait for servers to close

Consider refactoring to:

- after(() => {
-   servers.forEach(server => server.close());
- });
+ after(async () => {
+   await Promise.all(
+     servers.map(server => 
+       new Promise<void>((resolve, reject) => {
+         server.close(err => err ? reject(err) : resolve());
+       })
+     )
+   );
+ });

src/detect-port.ts Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/cli.test.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/detect-port.test.ts Show resolved Hide resolved
test/detect-port.test.ts Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (3)
package.json (1)

28-28: Consider using a lower @types/node version

The current @types/node version (^22.10.1) is unnecessarily high for Node.js 16+ support. Consider using a version that matches your minimum Node.js version requirement.

-    "@types/node": "^22.10.1",
+    "@types/node": "^16.18.0",
src/bin/detect-port.ts (2)

Line range hint 18-25: Consider using filter for array manipulation

The current implementation mutates the array and uses a loop with splice. Consider using filter for a more functional and efficient approach.

-const removeByValue = (arr: string[], val: string) => {
-  for (let i = 0; i < arr.length; i++) {
-    if (arr[i] === val) {
-      arr.splice(i, 1);
-      break;
-    }
-  }
-};
+const removeByValue = (arr: string[], val: string): string[] => {
+  return arr.filter(item => item !== val);
+};

Line range hint 35-77: Consider modernizing with async/await

The callback-based error handling could be modernized using async/await for better readability and error handling.

Example transformation:

try {
  const _port = await detectPort(port);
  if (isVerbose) {
    if (port !== _port) {
      console.log(`port ${port} was occupied`);
    }
    console.log(`get available port ${_port}`);
  } else {
    console.log(_port || port);
  }
} catch (err) {
  if (isVerbose) {
    console.log(`get available port failed with ${err}`);
  }
  process.exit(1);
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 815eb45 and 74f08a4.

📒 Files selected for processing (5)
  • package.json (1 hunks)
  • src/bin/detect-port.ts (4 hunks)
  • test/cli.test.ts (1 hunks)
  • test/detect-port.test.ts (1 hunks)
  • test/wait-port.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • test/wait-port.test.ts
  • test/detect-port.test.ts
  • test/cli.test.ts
🔇 Additional comments (3)
package.json (3)

58-69: Well-structured dual package exports configuration

The exports field is properly configured to support both ESM and CJS with their respective type definitions, following Node.js best practices for dual packages.


40-46: Well-structured TypeScript build and test scripts

The scripts are properly configured to handle TypeScript compilation, testing, and publishing workflows.


48-50: Node.js version requirement aligns with PR objectives

The engine field correctly reflects the breaking change of dropping support for Node.js versions below 16.

package.json Outdated Show resolved Hide resolved
src/bin/detect-port.ts Show resolved Hide resolved
@fengmk2 fengmk2 merged commit b5d32d2 into master Dec 8, 2024
7 checks passed
@fengmk2 fengmk2 deleted the liuhanqu-feat/ts branch December 8, 2024 03:48
fengmk2 pushed a commit that referenced this pull request Dec 8, 2024
[skip ci]

## [2.0.0](v1.6.1...v2.0.0) (2024-12-08)

### ⚠ BREAKING CHANGES

* Drop Node.js < 16 support

1. 使用 ts 重构
2. 使用 tshy 支持 esm 和 cjs
3. test 使用 test-runner (这里需要 node v18 版本)

merge from #51

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Introduced a new `waitPort` function to asynchronously wait for a
specified port to become available.
	- Added a new ESLint configuration to enforce TypeScript linting rules.

- **Bug Fixes**
	- Reverted a feature in the `detect-port` package due to issues raised.

- **Documentation**
	- Updated `README.md` for improved clarity and updated badge links.
	- Modified `CONTRIBUTING.md` to reflect changes in testing commands.

- **Chores**
	- Introduced a new TypeScript configuration file (`tsconfig.json`).
- Updated `package.json` to reflect changes in dependencies and project
structure.

- **Tests**
- Added comprehensive tests for the new `waitPort` and updated tests for
the CLI and `detectPort` function.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

### Features

* refactor with typescript to support esm and cjs both ([#56](#56)) ([b5d32d2](b5d32d2))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant